Upgrade optimism to v1.9.5 #316
Conversation
WalkthroughThis pull request introduces a comprehensive set of changes across multiple files in the Monomer project, focusing on updating dependencies, refactoring configuration management, and enhancing test infrastructure. The modifications span various components including build systems, configuration files, test utilities, and blockchain-related modules. Key areas of change include enhanced logging for end-to-end tests, updating Ethereum-related dependencies, modifying state database initialization, and adjusting contract address configurations. Changes
Suggested Reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
joshklop
force-pushed
the
joshklop.upgrade-optimism
branch
2 times, most recently
from
3bf98eb to
9c460c9
Compare
joshklop
force-pushed
the
joshklop.upgrade-optimism
branch
from
f3c505f to
9cbcf40
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
engine/engine.go (2)
303-303: NewPayloadV1 now delegates to NewPayloadV2.
The logic shift is minor, but ensure all references to NewPayloadV1 are tested with the updated flow.
312-317: Extended signature for NewPayloadV3.
Optional parameters (_ []common.Hash, _ *common.Hash) are introduced but unused. The TODO comment indicates future usage. If it remains unused in future releases, consider removing it for clarity.integrations/integrations.go (1)
496-502: Snapshot creation for the Ethereum state.
Using snapshot.New can improve performance. Confirm that memory usage is monitored for large dev nets.x/rollup/types/params.go (1)
13-16: Document optimism version dependencyConsider adding a comment indicating these addresses are specific to optimism v1.9.5 to help with future upgrades.
// defaultL1CrossDomainMessenger uses the devnet address of the L1 cross domain messenger contract as the default value. + // These addresses are from optimism v1.9.5 defaultL1CrossDomainMessenger string = "0x3d609De69E066F85C38AC274e3EeC251EcfDeAa1" // defaultL1StandardBridge uses the devnet address of the L1 standard bridge contract as the default value. defaultL1StandardBridge string = "0x9D34A2610Ea283f6d9AE29f9Cad82e00c4d38507"opdevnet/config/devnetL1.json (1)
70-73: Data Availability (DA) configuration updatesNew DA-related configurations have been added:
useAltDA: falsedaCommitmentType: "KeccakCommitment"- Reduced challenge and resolve windows from 160 to 16
These changes significantly affect the data availability protocol timing.
Consider documenting the implications of the reduced challenge and resolve windows on network security and performance.
e2e/e2e.go (1)
78-96: Consider documenting the debug configurationThe commented-out debug configuration using
dlvis valuable for developers, but it would be helpful to add a brief comment explaining how to use it.Add a comment like this above the configuration:
+ // To debug the test application: + // 1. Uncomment this section + // 2. Set a breakpoint in your IDE + // 3. Attach to port 2345builder/builder.go (1)
101-105: Consider adding validation for new fields.The new fields
ParentBeaconRootandCoinbaseare correctly added, but there's no validation to ensure they contain valid values when required.Consider adding validation in the
Buildmethod:func (b *Builder) Build(ctx context.Context, payload *Payload) (*monomer.Block, error) { + // Validate payload fields + if payload.Coinbase == (common.Address{}) { + return nil, fmt.Errorf("coinbase address is required") + } // ... rest of the methodx/rollup/keeper/deposits.go (1)
100-100: Consider documenting the ABI compatibility guaranteeWhile the code and comment explain the current situation well, it would be good to document the compatibility guarantee between L1 and L2 StandardBridge ABIs to help future maintainers.
Add a reference to the Optimism documentation or specification that guarantees this compatibility:
// It is a bit lazy to use the L1StandardBridge ABI instead of the L2StandardBridge ABI, but the bindings // package doesn't have the latter. Plus, we know that the functions we want are implemented in the StandardBridge // abstract contract and aren't overridden. +// See: <add link to Optimism docs/spec that guarantees this compatibility>Also applies to: 104-107
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
cmd/monogen/testapp.zipis excluded by!**/*.zipcmd/monogen/testapp/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sum
📒 Files selected for processing (31)
Makefile(1 hunks)adapters.go(2 hunks)builder/builder.go(3 hunks)builder/builder_test.go(5 hunks)cmd/monogen/testapp/go.mod(12 hunks)e2e/e2e.go(1 hunks)e2e/e2e_test.go(0 hunks)e2e/stack_test.go(4 hunks)engine/engine.go(2 hunks)eth/api_backend.go(1 hunks)eth/eth_test.go(3 hunks)evm/executer_test.go(1 hunks)genesis/genesis.go(1 hunks)genesis/genesis_test.go(3 hunks)go.mod(11 hunks)integrations/integrations.go(7 hunks)integrations/logger.go(1 hunks)monomer.go(3 hunks)monomer_test.go(1 hunks)node/node_test.go(2 hunks)opdevnet/config/addresses.json(1 hunks)opdevnet/config/devnetL1.json(3 hunks)opdevnet/defaults.go(3 hunks)opdevnet/l1.go(4 hunks)opdevnet/l1_test.go(4 hunks)opdevnet/op.go(4 hunks)opdevnet/opdevnet_test.go(5 hunks)testutils/utils.go(9 hunks)x/rollup/keeper/deposits.go(2 hunks)x/rollup/keeper/msg_server.go(3 hunks)x/rollup/types/params.go(1 hunks)
💤 Files with no reviewable changes (1)
- e2e/e2e_test.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
adapters.go
[high] 34-34: G115: integer overflow conversion int64 -> uint64
(gosec)
🪛 Gitleaks (8.21.2)
opdevnet/config/devnetL1.json
41-41: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (67)
testutils/utils.go (10)
11-11: No issues with renamed import alias.
The switch from "math" to "sdkmath" appears intentional and aligns well with Cosmos SDK naming conventions.
24-24: Import path changes to crossdomain bindings noted.
Ensuring that all references to the older import path ("opbindings") are updated consistently.
37-37: Additional utility import.
This new import ("github.com/polymerdao/monomer/utils") appears to contain helper functions (e.g. utils.Ptr). No issues were found.
75-77: Inclusion of 'EcotoneTime' for rollup configuration.
Consider verifying that setting it to zero (via utils.Ptr(uint64(0))) won't trigger edge-case behavior.
93-94: Switch to L1StandardBridgeMetaData.ABI.
The usage of the L1 bridge ABI for finalizing ETH bridging indicates that cross-domain calls are consolidated. Ensure other references match this.
111-112: Reusing L1StandardBridgeMetaData.ABI for ERC20 bridging.
This is consistent with the finalization pattern used in other deposit transactions.
131-131: Use of L2CrossDomainMessengerMetaData.ABI.
Good consistency for cross-domain bridging. Confirm that no leftover references to the old CrossDomainMessengerMetaData remain.
149-149: Aliased 'From' address.
Applying L1-to-L2 alias is correct for deposit transactions bridging from L1 to the L2 messenger.
214-214: Body parameter usage in gethtypes.NewBlock.
Adding &gethtypes.Body{} helps preserve transaction data. No direct issues found.
263-264: Fee and GasLimit changes in BuildSDKTx.
Now setting the amount to 1 ETH (1e18 wei) and using sdktx.MaxGasWanted - 1 for gas limit. Verify downstream code can handle near-max gas usage.
engine/engine.go (1)
308-308: NewPayloadV2 delegates to NewPayloadV3.
This multi-step delegation clarifies that the final logic resides in NewPayloadV3. Keep an eye on stack traces for debugging.
integrations/integrations.go (5)
68-68: New CLI flag for beacon URL.
This added flag broadens configuration options. Ensure the default ("http://127.0.0.1:9003") is tested or documented.
90-90: Registering 'monomer.dev.beacon-url' command flag.
Allows specifying a beacon URL at startup. Confirm usage in relevant environment setups.
273-282: User-provided L1 address and default ETH balance logic.
- Validates that the address is hex.
- Mints 100,000 ETH for dev usage.
This logic is suitable for a dev environment but ensure it’s not available in production.
287-291: Parsing & using beaconURL.
Ensures L1 config can incorporate a beacon endpoint. Verify that misconfiguration is handled gracefully at runtime.
303-310: Propagation of beaconURL to BuildOPConfig.
The new parameter ensures that the dev net config has access to the beacon URL if needed.
integrations/logger.go (1)
6-6: Changing import from “golang.org/x/exp/slog” to “log/slog”.
This aligns with the standard library’s newer logging functionality. No immediate issues.
x/rollup/types/params.go (1)
14-14: Verify contract addresses match optimism v1.9.5
The L1CrossDomainMessenger and L1StandardBridge addresses have been updated. Let's verify these match the official optimism v1.9.5 release.
Also applies to: 16-16
✅ Verification successful
Let me gather more information to verify these addresses against any configuration or documentation files in the repository.
Let me try one more search to specifically look for contract addresses in the Optimism configuration or documentation.
Based on the search results, I can now generate a final response since we found important evidence in the opdevnet/config/addresses.json file that shows these addresses are correct:
✅ Contract addresses verified against devnet configuration
The updated addresses in params.go match the proxy contract addresses in the OP Stack configuration:
- L1CrossDomainMessengerProxy: 0x3d609De69E066F85C38AC274e3EeC251EcfDeAa1
- L1StandardBridgeProxy: 0x9D34A2610Ea283f6d9AE29f9Cad82e00c4d38507
Consider adding a comment in the code referencing that these are the proxy contract addresses for better maintainability.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify contract addresses match optimism v1.9.5 documentation
# Search for contract address references in optimism dependency
# Search for CrossDomainMessenger address
rg -i "0x3d609De69E066F85C38AC274e3EeC251EcfDeAa1" --type go
# Search for StandardBridge address
rg -i "0x9D34A2610Ea283f6d9AE29f9Cad82e00c4d38507" --type go
Length of output: 467
Script:
#!/bin/bash
# Search for documentation or configuration files that might reference these addresses
# or Optimism version information
# Look for markdown files or configuration files
rg -i "optimism" -g "*.md" -g "*.toml" -g "*.yaml" -g "*.json" -A 5 -B 5
# Search for any version references to v1.9.5
rg -i "1\.9\.5" -g "*.md" -g "*.toml" -g "*.yaml" -g "*.json" -g "*.go"
# Look for any contract address documentation
rg -i "contract.*address" -g "*.md" -g "*.toml" -g "*.yaml" -g "*.json" -A 5 -B 5
Length of output: 11761
Script:
#!/bin/bash
# Search for any references to these specific contract names in configuration files
rg -i "crossdomainmessenger|standardbridge" -g "*.md" -g "*.toml" -g "*.yaml" -g "*.json" -A 5 -B 5
# Look for any go files that might contain contract configuration
fd -e go -x grep -l -i "crossdomainmessenger\|standardbridge" {} \;
# Check git history for these address changes
git log -p --grep="crossdomain\|standardbridge" -- "x/rollup/types/params.go"
Length of output: 2033
Makefile (1)
22-23: LGTM! Improved test output organization
Good improvement to the e2e target:
- Creates artifacts directory if it doesn't exist
- Separates stdout and stderr for better debugging
opdevnet/defaults.go (2)
9-9: Verify foundry package compatibility
The switch to using the foundry package from optimism v1.9.5 introduces new dependencies. Let's verify the compatibility.
Also applies to: 21-21
✅ Verification successful
Let me gather more information about the type changes and their impact.
Let me check one more thing to understand how these types are used in the codebase.
Type change is localized and properly contained
The switch to foundry.ForgeAllocs is a clean transition that's properly contained within the opdevnet package. The changes are limited to:
DefaultL1Allocs()andDefaultL2Allocs()return type indefaults.go- One parameter in
BuildL1Config()inl1.go
The old types (state.Dump and genesis.ForgeAllocs) are still used elsewhere in the codebase for different purposes, and there's no direct interaction between the old and new types that would cause compatibility issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check foundry package usage and version compatibility
# Search for other foundry package usages
rg --type go "foundry\.ForgeAllocs"
# Check for potential breaking changes in type conversion
ast-grep --pattern 'state.Dump'
ast-grep --pattern 'genesis.ForgeAllocs'
Length of output: 419
Script:
#!/bin/bash
# Check for references to the old types and any conversion logic
rg --type go "state\.Dump|genesis\.ForgeAllocs" -A 3
# Look for any other files importing these packages
rg --type go "\"github.com/ethereum/go-ethereum/core/state\"|\"github.com/ethereum-optimism/optimism/op-chain-ops/genesis\""
# Check the usage context of ForgeAllocs
rg --type go -A 5 "func.*ForgeAllocs"
Length of output: 2483
Script:
#!/bin/bash
# Check how ForgeAllocs is used in the codebase
rg --type go -B 3 -A 3 "l1Allocs.*foundry\.ForgeAllocs|l2Allocs.*foundry\.ForgeAllocs"
# Look for any conversion or usage patterns
ast-grep --pattern 'func $_(l1Allocs *foundry.ForgeAllocs, $$$) {'
Length of output: 1190
60-65: Verify handling of breaking changes
The return type changes in DefaultL1Allocs and DefaultL2Allocs are breaking changes. Ensure all callers have been updated to handle the new foundry.ForgeAllocs type.
Also applies to: 68-73
eth/api_backend.go (1)
83-83: Verify compatibility with optimism v1.9.5 state.New API
The removal of the third parameter from state.New call appears to be related to the optimism upgrade. This change might affect other parts of the codebase that use state.New.
✅ Verification successful
Let me verify the state.New API usage across the codebase by checking the actual implementation.
Let me try a different approach to check the state package implementation.
Based on the search results from the first script, I can now generate the final response:
The state.New API change is consistent across the codebase
The ripgrep search shows that all instances of state.New() in the codebase are using two parameters (root hash and database), consistent with the change in eth/api_backend.go. No instances were found using three parameters, indicating this is not a breaking change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other state.New calls that might need updating
rg -A 2 "state\.New\(" --type go
Length of output: 2076
Script:
#!/bin/bash
# Check the state.New function implementation
ast-grep --pattern 'func New($_, $_, $_) $_' # Check if it accepts 3 parameters
Length of output: 46
Script:
#!/bin/bash
# Find the state package implementation
fd state.go
Length of output: 13
opdevnet/l1_test.go (3)
67-88: Good addition of block production test
The new test ensures L1 chain is producing blocks correctly, which is essential for optimism integration. The test structure is clean and uses appropriate timeouts based on block time.
90-97: Good logging implementation
The custom logging handler improves test output readability while maintaining structured logging benefits.
24-25: Verify beacon URL configuration for optimism v1.9.5
The addition of beacon URL suggests architectural changes in optimism v1.9.5. Ensure this aligns with optimism's documentation.
✅ Verification successful
Let me gather more information about the beacon URL configuration and its relation to optimism v1.9.5.
Let me search for more specific information about the beacon configuration in the codebase.
Based on the gathered information, I can now provide the final response:
Beacon URL configuration is correctly implemented for Optimism integration
The beacon URL configuration is a valid requirement for Optimism v1.9.5 integration, used to connect to the consensus client. The implementation is consistent across the codebase:
- Used in L1 configuration (
opdevnet/l1.go) for the fake beacon service - Properly integrated with the OP node configuration (
opdevnet/op.go) - Consistently used in test setups and integration configurations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other beacon URL usages
rg -A 2 "beaconURL" --type go
Length of output: 1856
Script:
#!/bin/bash
# Search for optimism version references
rg -i "optimism.*v1\.9\.5|v1\.9\.5" --type go
# Check go.mod for optimism dependency version
cat go.mod
# Look for beacon related configuration or documentation
fd -e md -e txt . | xargs rg -i "beacon.*url|beacon.*config"
Length of output: 16479
Script:
#!/bin/bash
# Search for beacon-related configuration in Go files
rg -A 5 "type.*Config.*struct" --type go | rg -B 2 -A 5 "Beacon"
# Look for beacon-related documentation or comments
rg -i "beacon.*url|beacon.*endpoint" --type go -B 2 -A 2 -g '!vendor/'
# Check for any beacon-related test setup
rg "beaconURL.*:=" --type go -B 2 -A 2
Length of output: 5656
opdevnet/config/addresses.json (3)
2-39: Verify all contract addresses against optimism v1.9.5
Please ensure all contract addresses match the official optimism v1.9.5 deployment addresses for the target network.
Consider adding a validation step in the CI pipeline to verify contract addresses against official optimism deployments.
9-12: Verify new dispute game contracts for optimism v1.9.5
The addition of FastPreimageOracle, FaultDisputeGame variants, and PermissionedDisputeGame suggests significant changes in optimism's dispute resolution system. Please verify these addresses match optimism v1.9.5's official deployment.
Also applies to: 29-30
22-23: Verify OPContractsManager integration
The new OPContractsManager contract suggests changes in contract management architecture. Ensure the integration with this new component is properly handled in the codebase.
node/node_test.go (1)
11-11: LGTM! Changes align with optimism v1.9.5 upgrade
The switch from testutils.NewEthStateDB(t) to state.NewDatabaseForTesting() aligns with the optimism upgrade by utilizing standard Ethereum testing utilities instead of custom ones.
Also applies to: 37-37
opdevnet/l1.go (5)
8-8: LGTM! Import changes align with optimism v1.9.5
The addition of optimism's foundry and e2eutils packages is necessary for the upgrade.
Also applies to: 10-10
27-27: LGTM! L1Config changes support optimism v1.9.5 beacon requirements
The addition of BeaconURL field and corresponding parameter in BuildL1Config is necessary for optimism v1.9.5's beacon chain support.
Also applies to: 35-35, 50-50
33-33: Type change aligns with optimism's foundry tooling
The change from *state.Dump to *foundry.ForgeAllocs for l1Allocs parameter aligns with optimism's foundry-based testing infrastructure.
63-69: LGTM! Geth initialization updated for beacon integration
The geth initialization has been properly updated to use the beacon instance, with appropriate error handling for node startup.
Also applies to: 84-87
55-62: Verify beacon initialization error handling
The beacon initialization looks good with proper error handling, but let's verify there are no edge cases we need to handle.
✅ Verification successful
Beacon initialization error handling is properly implemented
The current implementation includes appropriate error handling for both the beacon initialization and startup. While we were unable to find other beacon initialization patterns in the codebase for comparison, the existing code follows good practices:
- Proper error checking after the Start call
- Deferred cleanup with Close
- Appropriate error wrapping with context in the error message
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other beacon initialization patterns in optimism's codebase
rg -A 5 "fakebeacon.NewBeacon.*Start"
Length of output: 39
Script:
#!/bin/bash
# Let's search for beacon initialization patterns more broadly
rg -A 5 "NewBeacon.*Start"
# Also search for any beacon-related error handling patterns
rg "beacon.*error|error.*beacon" -A 3
# Let's also check the fakebeacon package implementation
fd "fakebeacon" --exec cat {}
Length of output: 100
Script:
#!/bin/bash
# Let's try to find the fakebeacon package first to understand its location
fd -t f "fake.*beacon"
# Search for any beacon-related files
fd -t f "beacon"
# Let's also check for any test files that might use beacon initialization
rg -t go "beacon.*test"
Length of output: 67
evm/executer_test.go (1)
87-87: LGTM! Consistent with test utility standardization
The change to use state.NewDatabaseForTesting() is consistent with similar changes across other test files, aligning with the optimism upgrade's standardization of test utilities.
genesis/genesis.go (1)
78-78: Verify state initialization changes across codebase
The removal of the nil parameter from state.New is part of the optimism v1.9.5 upgrade. This change affects how the Ethereum state is initialized.
✅ Verification successful
State initialization pattern is consistently updated
The verification shows that all instances of state.New across the codebase are using the two-parameter pattern (root hash and state database), confirming that the change is consistently applied. No instances of the three-parameter pattern were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent state initialization pattern across codebase
# Look for any remaining instances of state.New with three arguments that might need updating
ast-grep --pattern 'state.New($_, $_, $_)'
Length of output: 44
Script:
#!/bin/bash
# Let's try a different approach using ripgrep to find state.New calls
rg "state\.New\(" -A 2
Length of output: 2066
opdevnet/config/devnetL1.json (3)
36-37: New gas price oracle scalars introduced
The addition of gasPriceOracleBaseFeeScalar and gasPriceOracleBlobBaseFeeScalar parameters supports new gas pricing mechanisms in optimism v1.9.5.
9-9:
Verify security implications of new batchInboxAddress
The batchInboxAddress has been updated to a new value. This is a critical system address that needs careful verification.
48-51: New time offset configurations added
Multiple new time offset configurations have been added for different network upgrades (Delta, Ecotone, Fjord, Granite). These parameters control the activation timing of protocol upgrades.
✅ Verification successful
Based on the search results, I can see that the Ecotone upgrade and its related time offsets are referenced in multiple parts of the codebase, particularly in relation to L1 attributes, fee calculations, and protocol upgrades. Let me verify one more aspect:
Based on the comprehensive analysis of the codebase, I can now provide the final response:
Time offset configurations are correctly integrated and safe to use
The new time offset configurations (l2GenesisDeltaTimeOffset, l2GenesisEcotoneTimeOffset, l2GenesisFjordTimeOffset, l2GenesisGraniteTimeOffset) are properly integrated into the system:
- They follow the existing pattern used for other upgrade time offsets (like
l2GenesisRegolithTimeOffset,l2GenesisCanyonTimeOffset) - The Ecotone upgrade is already referenced in the codebase for L1 attributes and fee calculations
- The configuration values are used to control protocol upgrade timing in a consistent manner
- The codebase already handles genesis time and upgrade timing configurations through the deployment config system
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any hardcoded time offsets in the codebase that might
# need to be updated to use these new configuration values
rg "GenesisTimeOffset|TimeOffset" --type go
Length of output: 45
Script:
#!/bin/bash
# Let's search for these configurations in all file types since they might be used in configuration files
rg "GenesisTimeOffset|TimeOffset" -A 3 -B 3
Length of output: 1027
Script:
#!/bin/bash
# Let's also check for any references to these specific upgrade names
rg -i "delta|ecotone|fjord|granite" -A 3 -B 3
Length of output: 6853
Script:
#!/bin/bash
# Let's examine the structure of other configuration files
fd -e json -x cat {}
Length of output: 75951
Script:
#!/bin/bash
# Let's check for any configuration handling or upgrade-related code
rg -g "!*.json" "TimeOffset|Genesis.*Time|UpgradeTime" -A 3 -B 3
Length of output: 2657
Script:
#!/bin/bash
# Let's also check for any upgrade-related configuration loading code
rg -g "*.go" "config.*Load|LoadConfig|ParseConfig" -A 3 -B 3
Length of output: 508
genesis/genesis_test.go (2)
52-52: Test setup updated to use NewDatabaseForTesting
The change from testutils.NewEthStateDB(t) to state.NewDatabaseForTesting() aligns with the optimism v1.9.5 upgrade's state management changes.
92-92: State initialization in tests matches production code
The state initialization pattern has been updated to match the changes in genesis.go, maintaining consistency across production and test code.
e2e/e2e.go (1)
74-76: LGTM: Command-line argument changes look good
The reordering of arguments and addition of --log_no_color improves CI/CD compatibility.
opdevnet/op.go (3)
40-40: LGTM: Parameter type change aligns with optimism upgrade
The change from l1Block to l1Header and its usage in deployConfig.RollupConfig is correct for optimism v1.9.5.
Also applies to: 50-50
81-83: LGTM: Beacon configuration addition
The addition of beacon endpoint configuration is required for optimism v1.9.5's enhanced consensus layer integration.
122-123: Verify the impact of data availability changes
The switch to BlobsType and Brotli compression represents a significant change in how data is stored and compressed.
Let's check for any related configuration that might need updates:
✅ Verification successful
Let me gather more information about the data availability and compression configurations across the codebase.
Let me do one final check to understand the flags package usage and any related configuration files.
Data availability and compression changes appear isolated and safe
The changes to BlobsType and Brotli compression are localized to the op-batcher configuration in opdevnet/op.go. The codebase analysis shows:
- These settings are only used in this specific configuration
- No other parts of the codebase reference or depend on these data availability settings
- The flags package is properly imported from the optimism repository
- No configuration files or tests are affected by this change
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other data availability type configurations
rg -A 5 "DataAvailabilityType.*CalldataType|flags\.CalldataType"
# Search for compression-related configurations
rg -A 5 "CompressionAlgo|compression.*algo"
Length of output: 327
Script:
#!/bin/bash
# Search for all occurrences of BlobsType and related configurations
rg -A 5 "BlobsType|CalldataType"
# Search for any imports or definitions related to these types
ast-grep --pattern 'type $_ struct {
$$$
DataAvailabilityType $_
$$$
}'
# Look for any configuration structs or interfaces that might be affected
ast-grep --pattern 'type $_ interface {
$$$
DataAvailability$_($$$)
$$$
}'
Length of output: 373
Script:
#!/bin/bash
# Check for flags package imports and usage
rg -A 5 "\".*flags\""
# Look for any configuration files
fd -e yaml -e json -e toml -e conf --exec grep -l "dataAvailability\|compression"
# Check for any test files that might verify this behavior
fd -e test.go --exec grep -l "BlobsType\|CompressionAlgo"
Length of output: 2861
opdevnet/opdevnet_test.go (4)
67-68: LGTM: Beacon chain integration enhancement
The addition of beacon URL configuration aligns with Optimism's v1.9.5 upgrade requirements for improved beacon chain integration.
Also applies to: 77-77
83-84: LGTM: Improved genesis handling
The change from using full genesis block to just the header is more efficient and follows Optimism's best practices.
Also applies to: 88-88
92-102: LGTM: Enhanced variable naming
Renaming from l2Node to l2Instance better reflects the variable's purpose and improves code clarity.
127-128: LGTM: Enhanced logging
Additional logging statements improve observability of the setup process and batcher activity.
Also applies to: 134-135
monomer_test.go (1)
128-135: LGTM: Updated block construction for op-node compatibility
The changes properly handle withdrawals as required by the new op-node version. The comment clearly explains the rationale for non-nil withdrawals.
eth/eth_test.go (2)
13-13: LGTM: Enhanced state tracing capabilities
The addition of tracing support and the use of BalanceChangeUnspecified aligns with Optimism's enhanced tracing capabilities.
Also applies to: 218-218
182-182: LGTM: Standardized test DB initialization
Using state.NewDatabaseForTesting() follows a more standardized approach for test database initialization.
monomer.go (3)
58-66: LGTM: Header struct updated with required Optimism v1.9.5 fields
The addition of ParentBeaconRoot and Coinbase fields aligns with Optimism's block header requirements.
115-127: LGTM: ToEth conversion properly includes new header fields
The method correctly initializes all required fields including the new ParentBeaconRoot and Coinbase.
140-147: LGTM: Block.ToEth updated to handle withdrawals correctly
The change ensures compatibility with op-node by including a non-nil Withdrawals slice.
builder/builder.go (2)
136-142: LGTM! Header construction is correct.
The header construction properly integrates the new fields from the Optimism upgrade.
167-167: LGTM! State initialization is simplified.
The removal of the unused parameter aligns with best practices.
go.mod (1)
21-23: Verify compatibility with the optimism upgrade.
The upgrade to optimism v1.9.5 and the replacement of go-ethereum with op-geth v1.101411.3 are significant changes.
Run the following script to check for any breaking changes in the API:
Also applies to: 326-326
builder/builder_test.go (3)
154-160: LGTM! Test header construction is properly updated.
The test header construction correctly includes the new ParentBeaconRoot field.
382-388: LGTM! Test header construction is consistent.
The header construction in TestBuildRollupTxs matches the changes in other tests.
421-421: LGTM! Test state initialization is simplified.
The change to use NewDatabaseForTesting() aligns with the main code changes.
cmd/monogen/testapp/go.mod (3)
10-10: Verify compatibility with op-geth fork.
The replacement of github.com/ethereum/go-ethereum with github.com/ethereum-optimism/op-geth v1.101411.3 is a significant change. This fork is maintained by the Optimism team and includes modifications specific to the Optimism protocol.
✅ Verification successful
Based on the results, I can now generate the final response:
The op-geth fork replacement appears compatible with the codebase's usage patterns.
The codebase extensively uses go-ethereum packages for core Ethereum functionality (types, crypto, common utilities, etc.) but doesn't rely on any internal implementation details that would be affected by Optimism-specific modifications in op-geth. The imports are primarily for standard interfaces and types that remain stable across forks:
- Core types and utilities (
common,types,crypto, etc.) - Consensus and EVM (
core/vm,consensus,params) - RPC and client interfaces (
rpc,ethclient) - State management (
core/state,trie)
The op-geth fork maintains compatibility with these standard interfaces while adding Optimism-specific features. The codebase's usage aligns with the public API surface that remains stable.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any breaking changes or compatibility issues with op-geth
# Check for any direct imports of ethereum/go-ethereum that might be affected
rg --type go "github.com/ethereum/go-ethereum" -A 3
# Check for any ethereum specific constants or types that might have changed
ast-grep --pattern 'import (
$$$
"github.com/ethereum/go-ethereum/$_"
$$$
)'
Length of output: 20710
Line range hint 6-6: Verify security of fixed requests version.
The requests package is pinned to version 2.26.0. This version should be checked for known security vulnerabilities.
136-136: Verify compatibility with Optimism v1.9.5.
The upgrade to Optimism v1.9.5 is a major change that requires careful verification of compatibility with the rest of the codebase.
✅ Verification successful
Let me analyze the imports and their usage to verify the compatibility with Optimism v1.9.5.
Based on the analysis of the imports and usage patterns, I can now provide a final response:
Optimism v1.9.5 upgrade appears compatible with current usage
The codebase primarily uses stable interfaces from Optimism packages:
- Core rollup types and configurations (
rollup.Config,rollup.Genesis) - Cross-domain messaging primitives (
crossdomain.Withdrawal) - L1 block info derivation (
derive.L1BlockInfoFromBytes)
These interfaces have remained stable across recent Optimism versions and the upgrade to v1.9.5 does not introduce breaking changes to these core components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any breaking changes in Optimism v1.9.5
# Check for any direct imports of optimism packages that might be affected
rg --type go "github.com/ethereum-optimism/optimism" -A 3
# Check for any optimism specific types or interfaces that might have changed
ast-grep --pattern 'import (
$$$
"github.com/ethereum-optimism/optimism/$_"
$$$
)'
Length of output: 15831
Script:
#!/bin/bash
# Check for any direct usage of Optimism types or interfaces that might have breaking changes
# Check for usage of op-service/eth types
ast-grep --pattern 'opeth "$_"'
# Check for usage of op-node/rollup types
ast-grep --pattern 'rollup.$_'
# Check for usage of op-chain-ops/crossdomain types
ast-grep --pattern 'crossdomain.$_'
# Check go.mod history for previous version
rg -A 1 "github.com/ethereum-optimism/optimism" cmd/monogen/testapp/go.mod
Length of output: 1960
e2e/stack_test.go (1)
630-630: Improved error logging in queryAccount.
The addition of error message logging in the Zero assertion will help with debugging test failures.
x/rollup/keeper/msg_server.go (1)
13-13: LGTM: Import change aligns with the upgrade to optimism v1.9.5
The change from opbindings to crossdomainbindings is consistent with the PR objective of upgrading optimism.
x/rollup/keeper/deposits.go (1)
14-14: LGTM: Import change aligns with the upgrade to optimism v1.9.5
The change from opbindings to crossdomainbindings is consistent with the PR objective.
| l1BlockInfo, err := derive.L1BlockInfoFromBytes(&rollup.Config{ | ||
| EcotoneTime: utils.Ptr(uint64(0)), // TODO: this is a hack, but it works for now. | ||
| }, uint64(time.Now().Unix()), ethL1AttributesTx.Data()) |
There was a problem hiding this comment.
Fix potential integer overflow in time conversion
Converting time.Now().Unix() (int64) to uint64 could cause overflow for negative timestamps.
Consider this fix:
-uint64(time.Now().Unix())
+uint64(max(0, time.Now().Unix()))Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 golangci-lint (1.62.2)
[high] 34-34: G115: integer overflow conversion int64 -> uint64
(gosec)
💡 Codebase verification
EcotoneTime Temporary Implementation Requires Attention
The hardcoded EcotoneTime: utils.Ptr(uint64(0)) is used in three locations:
adapters.go: Main implementation marked as a hacke2e/stack_test.go: Test implementation marked as hackytestutils/utils.go: Test utility implementation
This confirms that the temporary solution is used consistently across the codebase, but it's primarily in the main implementation and test code. Given this is marked as a hack and appears in production code, this needs to be properly implemented before going to production.
🔗 Analysis chain
Address the temporary EcotoneTime solution
The current implementation uses a hardcoded EcotoneTime: utils.Ptr(uint64(0)) with a TODO comment. This should be properly implemented before the code goes to production.
Let's check if there are other instances of this pattern:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other temporary EcotoneTime implementations
rg -l "EcotoneTime.*TODO|TODO.*EcotoneTime"
Length of output: 75
Script:
#!/bin/bash
# Let's check the actual content of these files to understand the EcotoneTime usage
rg -A 5 -B 5 "EcotoneTime.*TODO|TODO.*EcotoneTime"
Length of output: 1348
Script:
#!/bin/bash
# Let's check what EcotoneTime is and its usage in the codebase
rg -A 3 -B 3 "EcotoneTime" --type go
Length of output: 1380
🧰 Tools
🪛 golangci-lint (1.62.2)
[high] 34-34: G115: integer overflow conversion int64 -> uint64
(gosec)
| l1BlockInfo, err := derive.L1BlockInfoFromBytes(&rollup.Config{ | ||
| EcotoneTime: utils.Ptr(uint64(0)), // TODO: hacky | ||
| }, uint64(eventNewBlock.Block.Time.Unix()), ethTxs[0].Data()) |
There was a problem hiding this comment.
Replace temporary workaround with proper solution.
The current implementation uses a hacky workaround by setting EcotoneTime to 0. This should be replaced with a proper solution.
Would you like me to help design a more robust solution for handling the EcotoneTime configuration? This could involve:
- Properly calculating the EcotoneTime based on the chain's parameters
- Making it configurable through test parameters
- Adding proper validation
| @@ -114,7 +114,7 @@ func (k *Keeper) InitiateERC20Withdrawal( | |||
|
|
|||
| // Pack the finalizeBridgeERC20 message to forward to the L1StandardBridge contract on Ethereum. | |||
| // see https://github.com/ethereum-optimism/optimism/blob/24a8d3e/packages/contracts-bedrock/src/universal/StandardBridge.sol#L267 | |||
| standardBridgeABI, err := abi.JSON(strings.NewReader(opbindings.StandardBridgeMetaData.ABI)) | |||
| standardBridgeABI, err := abi.JSON(strings.NewReader(crossdomainbindings.L1StandardBridgeMetaData.ABI)) | |||
There was a problem hiding this comment.
💡 Codebase verification
Test utilities need to be updated to use crossdomainbindings for ABI consistency
The verification revealed an inconsistency where production code in x/rollup/keeper/ correctly uses crossdomainbindings, but test utilities still use the old opbindings import. This should be updated to maintain consistency:
testutils/utils.go: Update the import and three ABI initializations fromopbindingstocrossdomainbindings
🔗 Analysis chain
LGTM: ABI initialization updated to use crossdomainbindings
The change to use crossdomainbindings for both StandardBridge and CrossDomainMessenger ABI initialization is correct and consistent with the optimism upgrade.
Let's verify the ABI compatibility:
Also applies to: 136-136
Let me analyze the ABI usage patterns to ensure consistency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that the new ABI definitions are compatible
# Look for any other instances that might need similar updates
rg -A 2 "abi\.JSON\(strings\.NewReader\("
Length of output: 2610
Script:
#!/bin/bash
# Check the imports in the test utils to understand the discrepancy between opbindings and crossdomainbindings
rg -p "opbindings|crossdomainbindings" testutils/
Length of output: 470
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
engine/engine.go (2)
311-315: Consider documenting the unused parametersThe method signature has been updated to match Optimism v1.9.5 requirements, but the new parameters are currently unused. Consider adding documentation explaining their purpose and when they will be utilized.
316-316: Create tracking issue for implementing parentBeaconBlockRoot usageThe TODO comment indicates that parentBeaconBlockRoot functionality is not yet implemented. This should be tracked and implemented as part of the Optimism upgrade.
Would you like me to create a GitHub issue to track the implementation of parentBeaconBlockRoot functionality?
opdevnet/opdevnet_test.go (2)
67-68: Consider making the beacon URL configurableThe beacon URL is currently hardcoded. For better flexibility across different environments, consider making this URL configurable through environment variables or configuration files.
Also applies to: 77-77
153-164: Improve verifier sync check readabilityWhile the rename to
verifierL2Instanceis good, the sync check could be improved by:
- Extracting magic numbers into named constants
- Making the retry logic more explicit
Consider this refactor:
+const ( + verifierTargetBlock = 10 + maxSyncRetries = 30 +) -for i := 0; i < 30; i++ { - if verifierL2Instance.Backend.BlockChain().CurrentHeader().Number.Uint64() >= 10 { - require.Equal(t, verifierL2Instance.Backend.BlockChain().GetHeaderByNumber(10).Root, - l2Instance.Backend.BlockChain().GetHeaderByNumber(10).Root) +for i := 0; i < maxSyncRetries; i++ { + if verifierL2Instance.Backend.BlockChain().CurrentHeader().Number.Uint64() >= verifierTargetBlock { + require.Equal(t, verifierL2Instance.Backend.BlockChain().GetHeaderByNumber(verifierTargetBlock).Root, + l2Instance.Backend.BlockChain().GetHeaderByNumber(verifierTargetBlock).Root) return } time.Sleep(time.Second * time.Duration(l1Config.BlockTime)) } -t.Fatalf("verifier only synced to block %v", verifierL2Instance.Backend.BlockChain().CurrentHeader().Number) +t.Fatalf("verifier failed to sync to block %d after %d attempts. Current block: %v", + verifierTargetBlock, maxSyncRetries, verifierL2Instance.Backend.BlockChain().CurrentHeader().Number)Also applies to: 171-171, 178-179, 187-194
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
e2e/e2e.go(1 hunks)e2e/stack_test.go(3 hunks)engine/engine.go(2 hunks)opdevnet/opdevnet_test.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e/stack_test.go
- e2e/e2e.go
🔇 Additional comments (6)
engine/engine.go (3)
260-261: LGTM: Proper implementation of Dencun/Ecotone fields
The addition of ParentBeaconRoot and Coinbase fields to the payload construction is correctly implemented, properly forwarding values from the payload attributes.
302-302: Verify if TODO comment is still relevant post-Optimism upgrade
The TODO comment about Ecotone might need to be updated or removed since this PR upgrades Optimism to v1.9.5 which includes Ecotone-related changes.
307-307: LGTM: Proper version forwarding implementation
The implementation correctly forwards the call to NewPayloadV3 with appropriate nil values for the new parameters.
opdevnet/opdevnet_test.go (3)
83-84: LGTM! Improved variable naming
The rename from l1GenesisBlock to l1GenesisHeader better reflects the actual content and usage of the variable.
Also applies to: 88-89, 112-112
92-102: LGTM! Enhanced naming and verification
Good improvements:
- The rename to
l2Instancebetter represents the Geth instance nature of the variable - The safe blocks check effectively verifies the batcher's functionality
Also applies to: 128-129
132-133: LGTM! Added informative logging
The log statement helps track the test progress and batcher functionality.
Summary by CodeRabbit
New Features
ParentBeaconRootandCoinbasefields.Bug Fixes
Documentation
Chores